-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to LDK 0.0.123 #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, lets land once we have an RC.
Oh, wait, no, we need to replace our sweeper with the upstream one now. |
Oops! Will push an update for this. |
49bb446
to
ac6462f
Compare
src/main.rs
Outdated
@@ -1022,6 +1076,7 @@ async fn start_ldk() { | |||
} | |||
}); | |||
|
|||
// TODO: remove after a few months since the new `OutputSweeper` was added in LDK v0.0.123. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... actually I think we'll need migration code since outputs may remain spendable even if the user shuts down their node for a year. Can you confirm @TheBlueMatt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, do we want to bother with migration for the sample? Does the sample exist as a rolling sample or as a codebase for people to use? I guess we could leave the code here as-is and do the transition separately after we ask folks in the meeting on monday and on discord if anyone cares about migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sgtm. Thinking on it, seems more aligned with our goals to prioritize readability.
CI failed due to rust-lang/rust#63033, pushed a fixup. Not sure our stance on raising the MSRV of the sample but it seems the bug is fixed in 1.69. |
I feel fine with the fix, its not wildly intrusive, as long as we leave a comment linking to the rustc issue. |
63d5ee1
to
fcc899a
Compare
Added a comment for the rust bug and tweaked the TODO regarding |
fcc899a
to
af011c1
Compare
Updated from using the RC to the release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few nits.
@@ -1022,6 +1079,7 @@ async fn start_ldk() { | |||
} | |||
}); | |||
|
|||
// TODO: remove this, since the new `OutputSweeper` was added in LDK v0.0.123. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also modify the sweeper to read the stored SpendableOutputDescriptors
, have the OutputSweeper
track them and delete the entries. The corresponding migration logic is pretty trivial in LDK Node (see https://github.com/lightningdevkit/ldk-node/blob/640a1fdb7833ad9c74ede0926f990d85ac1b3bca/src/io/utils.rs#L240-L314), most of it is really error handling which we could just unwrap
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for pointing me to that code. I'm working on this, will open a follow-up based on this PR.
af011c1
to
c394d2f
Compare
Apologies @tnull I totally missed that review. Updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
No description provided.